Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add ray integration support (#2400) #2444

Merged
merged 21 commits into from
Aug 13, 2024

Conversation

glowskir
Copy link
Contributor

@glowskir glowskir commented Oct 14, 2023

Adds a basic instrumentation for the Ray framework (https://www.ray.io/)

Closes #2400

Documentation PR: getsentry/sentry-docs#11029

@leokster
Copy link

leokster commented May 2, 2024

Is it planned to merge this PR?

@szokeasaurusrex
Copy link
Member

@leokster Taking a look now – likely this PR will require some changes before we can merge

@szokeasaurusrex szokeasaurusrex force-pushed the ray-integration branch 3 times, most recently from 1e32127 to 44e3d24 Compare May 6, 2024 13:07
The integration includes performance support. Also, add tests for the integration.

Closes getsentry#2400
@glowskir
Copy link
Contributor Author

glowskir commented May 6, 2024

@szokeasaurusrex you can do the review, I will adjust accordingly

@smeubank
Copy link
Member

smeubank commented May 7, 2024

Might be interesting to review the PR for Langchain #2911

I am not familiar with Ray, but if it is a framework similar to Langchain, then following a similar model should be ideal

cc @colin-sentry if you have any thoughts on this integration

@szokeasaurusrex
Copy link
Member

Hey @glowskir, thanks for reaching out again and for contributing this PR.

We are currently working on an initiative to support AI tools in the Sentry SDK, and we would be interested in learning how you are using Ray and what you hope to be able to achieve by integrating Ray with Sentry.

If you are interested, we can set up a customer call with you and anyone else interested in seeing a Ray integration in Sentry, so that we can learn how you are using Ray and how a Ray integration in the SDK could help you. You can reach me at [email protected] – please let me know when you would have time.

@glowskir
Copy link
Contributor Author

@szokeasaurusrex This PR introduces RayIntegration to enable out of the box distributed tracing support. Ray has high level task interface and we want to see chain of tasks as single transaction in performance tab. At the moment of creating this PR there was no Opentelemetry support, now in newer versions of ray one can use Opentelemetry facitlities provided by Ray. I don't know how Sentry integrations relate to Opentelemetry hooks tbh, whether you are planning to adapt them or ignore them but this is a thing to consider.

@szokeasaurusrex
Copy link
Member

@glowskir, understood. In that case, we can help to get this PR merged.

Before I do an in depth review, I wanted to mention a few things that might be helpful here. First of all, since it looks like Ray is mostly used with AI, it might make sense to design this integration so it is compatible with our LLM Monitoring product. If you need some inspiration for how to do this, you can look at our existing Langchain integration, which works with the LLM Monitoring feature.

Additionally, I noticed while looking at your PR that you are using sentry_sdk.Hub in several places. sentry_sdk.Hub is deprecated as of 2.0.0, and although some of our existing code uses it, we are going to migrate all of these uses away from the Hub API and would like to avoid introducing any new Hub usages. Please see here for a guide of how to migrate Hub calls to the new Scope API.

Please let me know whether you have any questions! I am happy to help

@antonpirker antonpirker added the New Integration Integrating with a new framework or library label Jun 6, 2024
@antonpirker
Copy link
Member

Hi @glowskir , i tried to use your integration with a sample ray app (I have never used ray before)

Can you tell me how I can initialize sentry in the ray worker that is started when I call ray.init()? Is there an event/signal I can listen to?

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 2.81690% with 69 lines in your changes missing coverage. Please review.

Project coverage is 79.33%. Comparing base (2c1e31c) to head (4c5b16d).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Files Patch % Lines
sentry_sdk/integrations/ray.py 0.00% 69 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2444      +/-   ##
==========================================
- Coverage   79.71%   79.33%   -0.38%     
==========================================
  Files         132      133       +1     
  Lines       14264    14335      +71     
  Branches     3003     3013      +10     
==========================================
+ Hits        11370    11373       +3     
- Misses       2071     2140      +69     
+ Partials      823      822       -1     
Files Coverage Δ
sentry_sdk/consts.py 93.02% <100.00%> (+0.06%) ⬆️
sentry_sdk/integrations/ray.py 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

@antonpirker
Copy link
Member

Ok, got it to work. Did some more cleanup and the transactions/spans look nice:

Screenshot 2024-08-02 at 08 19 48

@antonpirker
Copy link
Member

This is the ray app I used for testing: https://github.com/antonpirker/testing-sentry/tree/main/test-ray

@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Aug 6, 2024
@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Aug 7, 2024
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Aug 8, 2024
@antonpirker
Copy link
Member

I fixed the crash when using Actors. Performance data is still not captured when using Actors but at least we are not crashing users code anymore. I also mention this in the docs, so everyone knows that Actors are not yet supported.

@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Aug 8, 2024
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Aug 9, 2024
@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Aug 9, 2024
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Aug 12, 2024
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See one comment, else LGTM

sentry_sdk/integrations/ray.py Outdated Show resolved Hide resolved
@sentrivana sentrivana added the Trigger: tests using secrets PR code is safe; run CI label Aug 12, 2024
@ZacharyHampton
Copy link

Thanks for the work on this ya'll, excited to use this when merged.

@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Aug 13, 2024
@antonpirker antonpirker enabled auto-merge (squash) August 13, 2024 11:59
@antonpirker antonpirker added the Trigger: tests using secrets PR code is safe; run CI label Aug 13, 2024
@antonpirker antonpirker merged commit 17a6cf0 into getsentry:master Aug 13, 2024
124 of 127 checks passed
arjennienhuis pushed a commit to arjennienhuis/sentry-python that referenced this pull request Sep 30, 2024
Adds a basic instrumentation for the Ray framework (https://www.ray.io/)

Closes getsentry#2400

----

Co-authored-by: Anton Pirker <[email protected]>
Co-authored-by: Ivana Kellyer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do not merge New Integration Integrating with a new framework or library Trigger: tests using secrets PR code is safe; run CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add Ray framework integration for sentry/apm.
7 participants